fix: patch removeEventListener to properly remove patched callbacks #158
Conversation
Codecov Report
@@ Coverage Diff @@
## master #158 +/- ##
==========================================
+ Coverage 94.06% 94.10% +0.04%
==========================================
Files 74 74
Lines 3536 3582 +46
Branches 374 387 +13
==========================================
+ Hits 3326 3371 +45
- Misses 210 211 +1
|
plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts
Outdated
Show resolved
Hide resolved
|
@rezakrimi the lint action is failing completely. Can you please look into this? |
Sure. |
|
Can you please provide more details what is the exact problem here ?, thx |
Sure thing! The added test shows that the addEventListener instrumentation actually breaks apps because it doesn't have the compensating logic for removeEventListener. A patched callback is actually added as the event listener, and when the app tries to remove the original one, it's not found/ignored, leaving our patched callback present (along with unexpected calls to the underlying original one). For some apps, this doesn't matter, but for others this can be arbitrarily bad (breaking functionality, thrown errors, infinite loops, etc. etc.). |
|
it looks fine, the failing build might be connected with yesterday eslint merge, we have to check it @dyladan https://circleci.com/gh/open-telemetry/opentelemetry-js-contrib/1546 |
It is definitely related. eslint isn't running at all because of a gts dependency error. |
yes the caching was wrong, I'll make a PR today to fix it |
plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts
Outdated
Show resolved
Hide resolved
|
#159 should fix the lint check. |
fix: fixing caching issue for lint action
|
@johnbley if you update from master now the lint should run properly |
Will do, but please hold on merging until I find the time to properly handle some edge cases better. |
|
Looks like the lint step in the build is failing again, but this time with out-of-date code... I ran lint:fix and pushed those changes. |
That's strange. Same thing happened to my PR on the main opentelemetry-js repo. For me it just got resolved after pushing another commit. |
| const listener = function () { | ||
| called = true; | ||
| }; | ||
| // add same listener three different ways |
There was a problem hiding this comment.
I'm curious about one scenario. To check that can you modify this a bit to test the case with overwriting the previous event with the same callback
let called2 = 0;
const listener2 = function () {
called2++;
};
document.body.addEventListener('bodyEvent', listener2);
document.body.addEventListener('bodyEvent', listener2);
document.body.addEventListener('bodyEvent', listener2);
// now fun
document.body.dispatchEvent(new Event('bodyEvent'));
assert.strictEqual(called2, 1);
There was a problem hiding this comment.
Yep, good catch and an easy fix for this one based on the tracking state. Pushing fix momentarily.
There was a problem hiding this comment.
There is one more thing a bonus :). Currently you can call addEventListener providing a 3rd param options. In options you can set once:true which means the event will be removed automatically. It might be worth to test that too.
There was a problem hiding this comment.
let called2 = 0;
const listener2 = function () {
called2++;
};
document.body.addEventListener('bodyEvent', listener2);
document.body.addEventListener('bodyEvent', listener2, { once: true });
// now fun
document.body.dispatchEvent(new Event('bodyEvent'));
assert.strictEqual(called2, 1);
document.body.dispatchEvent(new Event('bodyEvent'));
assert.strictEqual(called2, 1); // error ?
There was a problem hiding this comment.
and you can revert it add once first and then add without options, heh small things and many edge cases
There was a problem hiding this comment.
Argh. There's also the case that once:true shouldn't prevent us from allowing it to be adding later (it's not really a double-register). Will dive in and fix this one tomorrow. Thanks!
There was a problem hiding this comment.
thx for taking this into deep details :)
There was a problem hiding this comment.
Couldn't leave it alone; handled the once:true callbacks properly being removed when an event is dispatched.
plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts
Outdated
Show resolved
Hide resolved
plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts
Show resolved
Hide resolved
| ): boolean { | ||
| let listener2Type = this._wrappedListeners.get(listener); | ||
| if (!listener2Type) { | ||
| listener2Type = new Map(); |
There was a problem hiding this comment.
Is this going to be coerced into a WeakMap when you set it on _wrappedListeners?
There was a problem hiding this comment.
No, though making the HTMLElement reference weak is a good idea. Unfortunately I don't see a way to do it since there are no iterators/size/isEmpty methods available on WeakMap and without it we'd definitely leak. I'll ponder this but I'd like to get this fix in as it is since it's clearly better than the current breaking of functional behavior.
Thanks to all for spotting further issues and edge cases! |
We need to patch removeEventListener in order to compensate for wrapping callbacks passed to addEventListener.